Dev/2.3.0#65
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughPyMax 2.3.0 adds centralized ChangesPyMax 2.3.0 Feature Release
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/pymax/api/users/payloads.py (1)
30-37: ⚡ Quick winDefine explicit duplicate-phone behavior in
from_contacts.Line 32-37 currently overwrites earlier entries when two
ContactInfoitems share the samephone. Please make this explicit (reject duplicates or intentionally keep first/last) to avoid silent payload data loss.♻️ Proposed change
class ImportContactsPayload(CamelModel): contact_list: dict[str, _ContactPayload] # phone -> contact payload `@classmethod` def from_contacts(cls, contacts: Iterable[ContactInfo]) -> "ImportContactsPayload": - return cls( - contact_list={ - contact.phone: _ContactPayload( - first_name=contact.first_name, - ) - for contact in contacts - } - ) + contact_list: dict[str, _ContactPayload] = {} + for contact in contacts: + if contact.phone in contact_list: + raise ValueError(f"Duplicate phone in contacts: {contact.phone}") + contact_list[contact.phone] = _ContactPayload(first_name=contact.first_name) + return cls(contact_list=contact_list)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pymax/api/users/payloads.py` around lines 30 - 37, The from_contacts class method in ImportContactsPayload currently creates a dictionary with phone as the key, which silently overwrites earlier contact entries when duplicate phone numbers exist. Add explicit duplicate-phone handling logic before or within the dictionary comprehension to either reject duplicates by raising an exception, or intentionally keep the first or last occurrence of each phone number. Make your chosen behavior clear in the code with appropriate comments or conditional logic to prevent silent data loss.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pymax/api/chats/service.py`:
- Around line 367-377: The delete_chat method uses a truthiness check
(`last_event_time or int(time.time() * 1000)`) which incorrectly treats 0 as
falsy and replaces it with the current timestamp. Fix this by replacing the `or`
operator with an explicit `None` check using a conditional expression (ternary
operator or `if last_event_time is None else`) to allow 0 as a valid value for
the last_event_time parameter in the DeleteChatPayload instantiation.
In `@src/pymax/base.py`:
- Around line 277-279: The code closes the store via self.close() and then
attempts to call delete_session() on the already-closed store, followed by
calling store.close() again. Reorder the operations to call delete_session() on
the store before invoking self.close(), then remove the redundant await
store.close() call since self.close() already closes the store that was
captured. This ensures the session is deleted while the store is still open and
avoids mutating a closed resource.
In `@src/pymax/dispatch/router.py`:
- Line 35: Update the test type annotations in tests/app/test_app_runtime.py to
satisfy the new ClientT bound to BaseClient. Replace all instances of
Router[object] and App[object] at lines 138, 182-183, 310-311, 385, 400, and 432
with either Any (if you want to disable type checking for those tests) or a
concrete BaseClient subclass that properly implements the BaseClient interface.
This will resolve the type constraint violation introduced by the new TypeVar
bound in the router.py file.
- Around line 142-153: The on_error method in the router class accepts a scope
parameter that can be passed as a raw string (since ErrorScope subclasses str),
but the current implementation stores it as-is in the error_handlers list via
ErrorEntry. Downstream code uses identity comparison (is operator) to check
scope values, which fails for raw strings. Normalize the scope parameter by
wrapping it with ErrorScope(scope) before creating the ErrorEntry and appending
it to self.error_handlers to ensure identity checks work correctly and
LOCAL-scoped handlers are not incorrectly treated as GLOBAL.
In `@src/pymax/session/protocol.py`:
- Line 14: The `delete_all_sessions()` method should be removed from the
`StoreProtocol` interface definition in the protocol.py file. This method is not
required by the relogin path which only uses `delete_session()`, and adding it
to the protocol breaks backward compatibility with existing custom store
implementations. Keep the bulk-purge functionality as a concrete implementation
detail in specific store classes that need it, rather than as part of the
protocol contract.
---
Nitpick comments:
In `@src/pymax/api/users/payloads.py`:
- Around line 30-37: The from_contacts class method in ImportContactsPayload
currently creates a dictionary with phone as the key, which silently overwrites
earlier contact entries when duplicate phone numbers exist. Add explicit
duplicate-phone handling logic before or within the dictionary comprehension to
either reject duplicates by raising an exception, or intentionally keep the
first or last occurrence of each phone number. Make your chosen behavior clear
in the code with appropriate comments or conditional logic to prevent silent
data loss.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e288c93-8587-4413-a63f-64eb78981883
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
docs/api/router.rstdocs/chats.rstdocs/client.rstdocs/index.rstdocs/messages.rstdocs/release-2-3-0.rstdocs/router.rstdocs/types/contact_info.rstdocs/types/index.rstdocs/users.rstpyproject.tomlsrc/pymax/__init__.pysrc/pymax/api/chats/payloads.pysrc/pymax/api/chats/service.pysrc/pymax/api/messages/service.pysrc/pymax/api/users/payloads.pysrc/pymax/api/users/service.pysrc/pymax/app.pysrc/pymax/base.pysrc/pymax/dispatch/__init__.pysrc/pymax/dispatch/dispatcher.pysrc/pymax/dispatch/enums.pysrc/pymax/dispatch/router.pysrc/pymax/infra/chat.pysrc/pymax/infra/message.pysrc/pymax/infra/user.pysrc/pymax/session/protocol.pysrc/pymax/session/store.pysrc/pymax/types/domain/__init__.pysrc/pymax/types/domain/chat.pysrc/pymax/types/domain/message.pysrc/pymax/types/domain/user.pytests/api/test_chat_user_self_session_services.pytests/api/test_message_service.pytests/app/test_app_runtime.pytests/domain/test_bound_models.pytests/session/test_store.py
|
|
||
| _EventT = TypeVar("_EventT") | ||
| ClientT = TypeVar("ClientT") | ||
| ClientT = TypeVar("ClientT", bound="BaseClient") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find generic Router/App usages that violate ClientT's BaseClient bound.
# Expect: No Router[object] / App[object] usages remain, or they are intentionally changed to Any/concrete client types.
rg -nP -C2 '\b(Router|App)\s*\[\s*object\s*\]'Repository: MaxApiTeam/PyMax
Length of output: 2175
Update test type annotations to match the new BaseClient bound.
Multiple test helpers still use Router[object] and App[object] (lines 138, 182–183, 310–311, 385, 400, 432 in tests/app/test_app_runtime.py), which violate the ClientT bound to BaseClient. Either use Any for type annotations, provide a concrete BaseClient subclass, or reconsider the bound if these classes should accept arbitrary client types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pymax/dispatch/router.py` at line 35, Update the test type annotations in
tests/app/test_app_runtime.py to satisfy the new ClientT bound to BaseClient.
Replace all instances of Router[object] and App[object] at lines 138, 182-183,
310-311, 385, 400, and 432 with either Any (if you want to disable type checking
for those tests) or a concrete BaseClient subclass that properly implements the
BaseClient interface. This will resolve the type constraint violation introduced
by the new TypeVar bound in the router.py file.
Описание
Подготавливает релиз PyMax 2.3.0: добавляет release notes, поднимает версию пакета и дополняет документацию по новым публичным API.
В релиз вошли:
on_start-обработчиков;on_error()сErrorContextиErrorScope;on_disconnect();relogin();delete_chat()иChat.delete();import_contacts()иContactInfo;SessionStore.delete_all_sessions();edit_message()/Message.edit()наattachments=[...].Тип изменений
Связанные задачи / Issue
Ссылка на issue, если есть: #
Summary by CodeRabbit
Release Notes
New Features
on_starthandlers now execute after successful loginon_error()supporting global/local scopeson_disconnect()handler for connection state changesrelogin()method for session refresh and re-authenticationdelete_chat()import_contacts()Breaking Changes
attachmentsparameter (removed singularattachment)Documentation